Conversation
derive/src/lib.rs
Outdated
| // Remove any type parameter defaults, we don't want those. E.g. `impl<T: Stuff = WutEven>` | ||
| ast.generics.type_params_mut().for_each(|type_param| { | ||
| type_param.default = None; | ||
| }); |
There was a problem hiding this comment.
Is there a way to use ImplGenerics from split_for_impl() above to avoid this? Not sure whether it will work with our modified lifetimes or not.
There was a problem hiding this comment.
You are correct that ImplGenerics takes care of removing the defaults and it takes care of const parameter defaults too, so it's strictly better.
However there's another problem: we replace all lifetimes with 'static and this means that given this input:
struct Goat<'bar, T: SomeTrait = SomeDefault>
we get this in the expansion:
impl<'static, T: SomeTrait>
(which is invalid syntax)
So ImplGenerics is the right tool for the job, but using it will require cloning&munging elsewhere. Not sure what is least worse tbh. :/
There was a problem hiding this comment.
I have an idea: remove the replacing of lifetimes and add something like this to make_where_clause:
for lifetime in generics.lifetimes() {
where_clause
.predicates
.push(parse_quote!(#lifetime : 'static));
}
That way we should be able to use ImplGenerics and remove completely the two mutable bits.
There was a problem hiding this comment.
You are completely right. And as an extra bonus we now don't need to clone the input anymore. ❤️
There was a problem hiding this comment.
With your suggestion we get an expansion like this:
struct Assoc<'bar, T: Types> {
a: T::A,
b: &'bar u64,
}
const _: () = {
impl<'bar, T: Types> ::scale_info::TypeInfo for Assoc<'bar, T>
where
'bar: 'static,
T::A: ::scale_info::TypeInfo + 'static,
T: Types + ::scale_info::TypeInfo + 'static,
…
…That 'bar: 'static looks a bit funny but it's valid syntax so all good.
Avoid cloning&parsing the ast
derive/src/lib.rs
Outdated
| // limitations under the License. | ||
|
|
||
| #![cfg_attr(not(feature = "std"), no_std)] | ||
| // #![cfg_attr(not(feature = "std"), no_std)] |
There was a problem hiding this comment.
Ouch, my bad.
But while we're at it: we should probably remove this anyway. The derive crate doesn't need to be no-std I think (only its output must be no-std). And as we're using proc-macro-crate already it's perhaps good to acknowledge the fact that scale-info-derive isn't actually no-std?
There was a problem hiding this comment.
Yeah I'm not sure why that's there in the first place, but if we remove it should not be hidden in this PR
There was a problem hiding this comment.
See #69 (comment) – I removed it in that PR.
Defaults for type parameter bounds are only allowed in structs, enums, type and trait definitions, not in impl blocks, so when deriving the
TypeInfoimpl we have to remove them.See rust-lang/rust#36887 for details.